Prefer cached encryptionKey for encryption when possible
#307
Conversation
| @@ -930,6 +930,7 @@ class KeyringController extends EventEmitter { | |||
|
|
|||
| if ( | |||
| this.password && | |||
| (!this.#cacheEncryptionKey || !encryptionKey) && | |||
There was a problem hiding this comment.
This ensures that we don't do the whole persistAllKeyrings operation for nothing, as the vault will not get updated if we reuse the cached encryption key
mikesposito
changed the title
encryptionKey for encryption when possibleencryptionKey for encryption when possible
| }, | ||
| }); | ||
| await keyringController.createNewVaultAndKeychain(PASSWORD); | ||
| deleteEncryptionKeyAndSalt(keyringController); |
There was a problem hiding this comment.
Ah I see; this method is needed because this kinda isn't a real use-case. Or it is, but only as part of the initial unlock/vault creation, it's never a use case when calling persistAllKeyrings directly.
Makes sense. Seems like a bit of a code smell that this condition is possible only when this external method is called in a certain way internally, but that problem will go away soon hopefully when we merge this into core.
There was a problem hiding this comment.
A nicer way to test this would be to test it as part of createNewVaultAndKeychain (and the other methods that trigger this condition), but given that this is to be deleted soon I don't think it'd be worth the effort 😅
|
|
||
| expect(initialVault).not.toBe(updatedVault); | ||
| expect(updatedVault).toBe(updatedVaultMock); | ||
| describe(`with cacheEncryptionKey = true and encryptionKey is unset`, () => { |
There was a problem hiding this comment.
Nit: Is there a test case for when cacheEncryptionKey = true and encryptionKey is set?
There was a problem hiding this comment.
Added in fe45b61!
The test checks that the vault is not being updated
* fix: prefer encryptionKey for encryption when possible * refactor: add test case
* [15.x] Backport encryption improvements (#312) * Prefer cached `encryptionKey` for encryption when possible (#307) * fix: prefer encryptionKey for encryption when possible * refactor: add test case * Use encryptor `isVaultUpdated` (#310) * chore: update browser-passworder * refactor: remove `updateVault` from `GenericEncryptor` * 15.1.0 (#313) * 15.1.0 * edit changelog entries * rephrase changelog entry --------- Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: Michele Esposito <michele@esposito.codes> --------- Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: Michele Esposito <michele@esposito.codes> Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Description
When persisting keyrings,
EthKeyringControlleralways prefers the use of the password over the cached encryption key. This means that we'll always derive the password (which is an heavy task), even if we already have the key that we need.This PR changes the order of the check, putting the
encryptionKeyon top. Some changes were required on some tests after that as well.Changes
References
keyring-controllerandbrowser-passwordermetamask-extension#21878 (comment)Checklist